packages/intent/src/core/lockfile/lockfile-state.ts (1)

17-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the suffix-matching heuristic in packageRelativeSkillFile.

The for loop that walks packageSegments looking for the longest suffix matching a prefix of skillSegments is correct for the tested shapes (bare node_modules/<pkg>/... and full workspace-relative paths) but is non-obvious. Since this function determines the canonical skill path baked into intent.lock (the security-relevant acceptance record), a short comment explaining why it scans from start=0 upward (to prefer the longest/most-specific suffix match) would reduce the risk of a subtly wrong edit later.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/core/lockfile/lockfile-state.ts` around lines 17 - 52,
Document the suffix-matching heuristic in packageRelativeSkillFile immediately
above the loop: explain that scanning packageSegments from start=0 upward checks
progressively shorter suffixes while preferring the longest, most-specific
suffix matching the skillSegments prefix, preserving canonical lockfile path
resolution.

packages/intent/src/core/lockfile/lockfile.ts (1)

156-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

readIntentLockfile bypasses the injectable-fs pattern used elsewhere in this PR.

Unlike computeSkillContentHash, buildCurrentLockfileSources, and scanIntentPackageAtRoot, which all accept a ReadFs/fsCache, readIntentLockfile calls readFileSync from node:fs directly. Its two call sites — intent-core.ts's toResolvedIntentSkill and catalog-lock.ts's applyCatalogueLock — both otherwise thread an injected/cached readFs through the rest of the same function, so this is the one lockfile read that always hits real disk regardless of caching. Consider accepting an optional fs/readFs parameter (defaulting to real readFileSync) for consistency and to avoid an uncached disk read + JSON parse on every skill resolution.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/core/lockfile/lockfile.ts` around lines 156 - 167, Update
readIntentLockfile to accept the injected ReadFs/fsCache reader used by
computeSkillContentHash, buildCurrentLockfileSources, and
scanIntentPackageAtRoot, while defaulting to the real filesystem reader for
existing callers. Thread the available readFs through intent-core.ts’s
toResolvedIntentSkill and catalog-lock.ts’s applyCatalogueLock so lockfile reads
use the same cache instead of direct readFileSync.

packages/intent/tests/core.test.ts (1)

541-571: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a resolveIntentSkill assertion for parity with the sibling tests.

The two adjacent tests (content-hash mismatch, missing lock entry) both assert loadIntentSkill and resolveIntentSkill throw identically. This test only checks loadIntentSkill, leaving the wrong-source-kind path unverified for resolveIntentSkill, even though both share the same toResolvedIntentSkill check.

✅ Proposed addition

     expect(() =>
       loadIntentSkill('`@tanstack/query`#fetching', { cwd: root }),
     ).toThrow(
       'Cannot load skill use "`@tanstack/query`#fetching": skill is not accepted in intent.lock.',
     )
+    expect(() =>
+      resolveIntentSkill('`@tanstack/query`#fetching', { cwd: root }),
+    ).toThrow(
+      'Cannot load skill use "`@tanstack/query`#fetching": skill is not accepted in intent.lock.',
+    )
   })

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/tests/core.test.ts` around lines 541 - 571, Extend the
wrong-source-kind test to also assert that resolveIntentSkill rejects the same
`@tanstack/query`#fetching request with the identical intent.lock error, matching
the adjacent content-hash and missing-entry tests. Keep the existing
loadIntentSkill assertion unchanged and reuse the same root and fixture setup.

packages/intent/src/discovery/scanner.ts (1)

169-171: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Memoize per root — each call re-executes the PnP runtime.

loadPnpApi requires and runs .pnp.cjs setup(), which patches the global fs module process-wide. getProjectReadFs is invoked at least once from getIntentCatalogContext and again as the default argument in applyCatalogueLock, so a single intent catalog run can load and set up the PnP runtime twice. The scanner already caches this lazily via its internal pnpApi variable; a small module-level Map<string, ReadFs> here would give the same guarantee to external callers.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/discovery/scanner.ts` around lines 169 - 171, Memoize
getProjectReadFs results per root to avoid repeated PnP runtime setup. Add a
module-level Map<string, ReadFs>, return the cached filesystem when available,
and cache either the loaded PnP readFs or nodeReadFs before returning from
getProjectReadFs.

packages/intent/src/core/source-policy.ts (1)

199-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Discovery warnings are dropped here.

scanForIntents returns warnings (invalid intent fields, version conflicts) which scanForConfiguredIntents discards, so intent sync can silently ignore real discovery problems while only surfacing policy.notices. Consider returning warnings alongside discovered/policy so the sync output can include them.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/core/source-policy.ts` around lines 199 - 207, Update
scanForConfiguredIntents to preserve and return the warnings from scanForIntents
alongside discovered and policy. Ensure the intent sync output can consume these
discovery warnings in addition to policy.notices, without filtering or replacing
the existing warning details.

packages/intent/tests/cli.test.ts (1)

291-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Clear logSpy between lifecycle phases.

Each assertion joins the cumulative call buffer, so output emitted by an earlier main(['sync']) in this test keeps satisfying later toContain checks. If a phase stops printing its block, the test can still pass. Add logSpy.mockClear() immediately before each main(['sync']) whose output you then assert.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/tests/cli.test.ts` around lines 291 - 338, Clear the
cumulative log buffer before each lifecycle phase in the test’s sync flow: call
logSpy.mockClear() immediately before every main(['sync']) invocation whose
output is subsequently asserted. Keep the existing assertions and sync behavior
unchanged while ensuring each check validates only that phase’s log output.

packages/intent/src/session-catalog.ts (2)

310-317: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Bound the upward walk.

while (directory !== workspaceRoot) relies on exact string equality to terminate; any normalization drift between policyRoot and workspaceRoot (trailing separator, case on Windows) turns this into an infinite loop, since dirname becomes a fixed point at the filesystem root. Add a depth/dirname fixed-point guard.

🛡️ Proposed guard

   const manifests: Array<string> = []
   let directory = policyRoot
   while (directory !== workspaceRoot) {
     manifests.push(join(directory, 'package.json'))
-    directory = dirname(directory)
+    const parent = dirname(directory)
+    if (parent === directory) break
+    directory = parent
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/session-catalog.ts` around lines 310 - 317, Bound the
upward manifest walk by updating the loop around directory and workspaceRoot to
stop when the current directory reaches workspaceRoot or dirname(directory)
becomes unchanged at the filesystem root. Preserve collecting package.json for
each traversed directory and avoid adding entries indefinitely when path
normalization differs.

---

249-266: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Fingerprint reads full lockfile contents on every hook invocation.

hash.update(readFileSync(file)) slurps every lockfile (pnpm-lock.yaml, package-lock.json, …) plus every workspace package.json on each getSessionCatalogue call, including cache hits. In a large monorepo that is tens of MB of I/O inside a session-start hook that has a 10s timeout. Hashing size + mtimeMs from statSync (falling back to content only when needed) would give the same invalidation signal far more cheaply.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/session-catalog.ts` around lines 249 - 266, Update the
fingerprinting loop in the session-catalog fingerprint function to avoid reading
full lockfiles and workspace package.json files on every invocation. Use each
file’s stat metadata, including size and modification time, as the primary hash
input, and fall back to hashing file contents only when stat metadata cannot be
obtained; preserve the existing missing-file handling and deterministic path
ordering.

packages/intent/tests/catalog-api.test.ts (1)

72-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Tests leak session-cache files into the OS temp dir.

getIntentCatalogContext writes to the default os.tmpdir()/tanstack-intent/catalogues and afterEach only removes the fixture roots, so every run leaves orphaned cache JSON behind. Threading a cacheDir option through getIntentCatalogContext (already supported by getSessionCatalogue) would let these tests point at a fixture-local directory and clean up deterministically.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/tests/catalog-api.test.ts` around lines 72 - 77, Update the
test setup around getIntentCatalogContext to provide a fixture-local cacheDir,
using the existing roots-based temporary directory rather than the default OS
temp location. Ensure the option is threaded through the context creation to
getSessionCatalogue, and retain the afterEach cleanup so the cache files are
removed with the fixture roots.

packages/intent/tests/lockfile.test.ts (1)

73-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert on the rejection reason, not just that something throws.

All seven cases use bare toThrow(), so a parse failure for an unrelated reason (or a future message change that collapses two distinct validations into one) still passes. Since this is the fail-closed path for lockfile integrity, match the expected messages, e.g. toThrow(/source kind/) for the "kind":"git" case and /duplicate/i for the duplicate id/path cases.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/tests/lockfile.test.ts` around lines 73 - 103, Update the
rejection assertions in the lockfile validation test to match each case’s
expected error message, rather than using bare toThrow() calls. Anchor the
expectations to the relevant validation symbols and messages: unknown top-level
fields, unsupported lockfileVersion, invalid sources type, invalid source kind,
duplicate source identity, duplicate skill paths, and traversal paths should
each assert a targeted message; use patterns such as /source kind/ and
/duplicate/i where appropriate.

packages/intent/src/hooks/install.ts (2)

428-437: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename isIntentGateScriptReference now that it matches catalog scripts too.

The predicate no longer identifies only gate scripts; isIntentHookScriptReference reflects the widened regex.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/hooks/install.ts` around lines 428 - 437, Rename
isIntentGateScriptReference to isIntentHookScriptReference throughout its
declaration and all call sites, preserving the existing regex and behavior that
matches both gate and catalog scripts.

---

354-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid materializing an empty PreToolUse key in agent configs.

removeIntentHooks(arrayValue(hooks.PreToolUse)) always assigns an array, so a config that never had PreToolUse gains "PreToolUse": [] (and a user whose only Intent hook is removed is left with an empty array). Drop the key when the result is empty to keep the written config clean.

♻️ Proposed cleanup helper

-  hooks.PreToolUse = removeIntentHooks(arrayValue(hooks.PreToolUse))
+  const preToolUse = removeIntentHooks(arrayValue(hooks.PreToolUse))
+  if (preToolUse.length > 0) hooks.PreToolUse = preToolUse
+  else delete hooks.PreToolUse

Also applies to: 377-377, 389-390

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/hooks/install.ts` at line 354, Update the hook cleanup
assignments around hooks.PreToolUse so empty results do not create or retain a
PreToolUse key; only assign the filtered array when non-empty, otherwise remove
the property. Apply the same behavior to the additional affected hook-processing
paths while preserving existing handling for non-empty hooks.

packages/intent/src/commands/sync/gitignore.ts (1)

12-15: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Hoist the block matcher to a module constant.

START/END are static, so the escaping and RegExp construction can be computed once instead of on every call. (The ReDoS hint from static analysis is a false positive here — the pattern derives from constants and is escaped.)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/commands/sync/gitignore.ts` around lines 12 - 15, Hoist
the escaped START/END block matcher from the current function into a
module-level constant, preserving the existing non-greedy pattern and
replacement behavior in the sync logic. Remove the per-call RegExp construction
while continuing to use the shared matcher for prefix replacement.

Source: Linters/SAST tools

packages/intent/src/commands/sync/command.ts (1)

90-97: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Use writeTextFileAtomic for the .gitignore update.

Every other managed file in this module (package.json, install state) is written atomically; a plain writeFileSync here can truncate a user's .gitignore if the process dies mid-write.

♻️ Proposed fix

-  writeFileSync(path, after, 'utf8')
+  writeTextFileAtomic(path, after)
   return true

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/commands/sync/command.ts` around lines 90 - 97, Update
writeGitignore to replace the direct writeFileSync call with the module’s
existing writeTextFileAtomic helper, preserving the current before/after
comparison and boolean return behavior.

packages/intent/src/commands/sync/prepare.ts (2)

16-20: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Idempotency only holds for &&-chained scripts.

A prepare value like "build; intent sync" or "pnpm exec intent sync" won't be detected, so a second wiring appends a duplicate && intent sync. Splitting on [;&]+ (and optionally allowing a runner prefix) would cover the common shapes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/commands/sync/prepare.ts` around lines 16 - 20, Update
containsIntentSync to recognize intent sync commands separated by both && and
semicolons, and allow common runner prefixes such as pnpm exec before intent
sync. Preserve whitespace handling and ensure these command forms are detected
so repeated wiring remains idempotent.

---

33-42: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

Non-string existing prepare is silently overwritten.

If scripts.prepare is present but not a string (malformed manifest), the edit replaces it with "intent sync" without warning. Consider failing loudly like the parse-error path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/src/commands/sync/prepare.ts` around lines 33 - 42, Update
the prepare-edit logic around scripts.prepare to detect a present non-string
value and fail loudly using the same error behavior as the parse-error path,
rather than replacing it with "intent sync". Preserve the existing handling for
missing, string, and intent-sync-containing prepare values.

packages/intent/package.json (1)

20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Put types before import in the new ./catalog condition block.

Conditional exports are matched in declaration order; TypeScript's node16/bundler resolvers can match import first and only find catalog.d.mts via sibling-name fallback. Ordering types first makes declaration resolution explicit and consistent with the other subpaths.

♻️ Proposed ordering fix

     "./catalog": {
-      "import": "./dist/catalog.mjs",
-      "types": "./dist/catalog.d.mts"
+      "types": "./dist/catalog.d.mts",
+      "import": "./dist/catalog.mjs"
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/package.json` around lines 20 - 22, Reorder the conditions in
the "./catalog" export block so the existing "types" entry appears before
"import", matching the ordering used by other subpaths and ensuring declaration
resolution selects catalog.d.mts explicitly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/intent/tests/integration/source-policy-surfaces.test.ts (1)

90-105: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Also cover agent-facing warning and source redaction.

The result exposes warnings and hiddenSources in addition to packages and notices. Assert that UNLISTED is absent from warnings and that hiddenSources is empty for audience: 'agent'; otherwise a privacy regression could pass this test.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/intent/tests/integration/source-policy-surfaces.test.ts` around
lines 90 - 105, Extend the agent-audience test around listIntentSkills to assert
that no warning contains UNLISTED and that result.hiddenSources is empty. Keep
the existing package and notice assertions unchanged.